feat: display juju debug-log in integration test runs#360
feat: display juju debug-log in integration test runs#360PietroPasotti wants to merge 27 commits intomainfrom
Conversation
lucabello
left a comment
There was a problem hiding this comment.
To re-review this PR, I would like:
- convert the PR title to conventional commits format, like we practice everywhere
- test the branch with some repository (if you don't know which one, use https://github.com/canonical/o11y-tester-operator) and link a successful run here
- consider updating the diagram here and see how it looks like (adding another box under integration tests maybe)?
|
@lucabello ready for another round, all points addressed |
|
@copilot please make comment suggestions that would make the workflows lint pass. |
Agent-Logs-Url: https://github.com/canonical/observability/sessions/f549ed24-695e-4774-81db-5a5c87f2f1a5 Co-authored-by: lucabello <36242061+lucabello@users.noreply.github.com>
Fixed in 07b4840. The issue was a shellcheck SC2193 warning on the |
There was a problem hiding this comment.
Pull request overview
This PR extends the charm CI reusable workflows to surface integration test logs in GitHub Actions output and as downloadable artifacts, making debugging (e.g., Juju debug-log) easier across charm repositories.
Changes:
- Adds a new
log-pathworkflow input (default.logs) to control where integration test logs are collected from. - Updates the quality-checks workflow to display
*.txtlogs in the job output and upload the log directory as an artifact. - Updates the README Mermaid workflow diagram to include the new log display/upload steps.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| README.md | Documents the additional “Display logs” / “Upload logs” steps in the workflow diagram. |
| .github/workflows/charm-pull-request.yaml | Introduces log-path input and forwards it into the reusable quality-checks workflow. |
| .github/workflows/_charm-quality-checks.yaml | Adds log-path input and implements log display + upload behavior for integration runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log-path: | ||
| type: string | ||
| description: | | ||
| Path to a directory where the .txt logfiles for an integration testing run will be stored. |
There was a problem hiding this comment.
Spelling: use "log files" instead of "logfiles".
| Path to a directory where the .txt logfiles for an integration testing run will be stored. | |
| Path to a directory where the .txt log files for an integration testing run will be stored. |
| description: | | ||
| Path to a directory where the .txt logfiles for an integration testing run will be stored. | ||
| Relative and absolute file paths are both allowed. | ||
| Relative paths are rooted against the charm repository root. |
There was a problem hiding this comment.
The log-path description says relative paths are rooted against the "charm repository root", but the called workflow’s implementation prepends inputs.charm-path for relative paths. Please align this description with the actual behavior so callers don’t misplace logs.
| Relative paths are rooted against the charm repository root. | |
| Relative paths are resolved relative to `charm-path`. |
| pack["Pack the charm"] | ||
| integration["Integration tests"] | ||
| displaylogs["Display logs"] | ||
| uploadlogs["Upload logs zipfile"] |
There was a problem hiding this comment.
Spelling: "zipfile" → "zip file" in the diagram label.
| uploadlogs["Upload logs zipfile"] | |
| uploadlogs["Upload logs zip file"] |
| log-path: | ||
| type: string | ||
| description: | | ||
| Path to a directory where the .txt logfiles for an integration testing run will be stored. | ||
| Relative and absolute file paths are both allowed. | ||
| Relative paths are rooted against the charm repository root. | ||
| Paths that begin with a wildcard character should be quoted to avoid being | ||
| interpreted as YAML aliases. | ||
| required: false | ||
| default: .logs |
There was a problem hiding this comment.
Security: the workflow later interpolates ${{ inputs.log-path }} / derived outputs directly into run: bash scripts. If a caller supplies a value containing $()/backticks/newlines, it can lead to script-injection. Prefer passing the input via env: (e.g., LOG_PATH: ${{ inputs.log-path }}) and then using "$LOG_PATH" inside the script, which treats the value as data rather than code.
| log-path: | ||
| type: string | ||
| description: | | ||
| Path to a directory where the .txt logfiles for an integration testing run will be stored. | ||
| Relative and absolute file paths are both allowed. | ||
| Relative paths are rooted against the charm repository root. | ||
| Paths that begin with a wildcard character should be quoted to avoid being | ||
| interpreted as YAML aliases. | ||
| required: false | ||
| default: .logs |
There was a problem hiding this comment.
The log display/upload steps appear to be added only to the parallel integration job. If parallelize-integration is disabled (sequential integration run), logs won’t be shown or uploaded; consider applying the same log handling to the sequential integration job as well so behavior is consistent across integration modes.
| log-path: | ||
| type: string | ||
| description: | | ||
| Path to a directory where the .txt logfiles for an integration testing run will be stored. |
There was a problem hiding this comment.
Spelling: use "log files" instead of "logfiles".
| Path to a directory where the .txt logfiles for an integration testing run will be stored. | |
| Path to a directory where the .txt log files for an integration testing run will be stored. |
| displaylogs["Display logs"] | ||
| uploadlogs["Upload logs zipfile"] |
There was a problem hiding this comment.
In the Mermaid workflow diagram, displaylogs and uploadlogs are added but not connected to the existing flow. Consider adding edges (e.g., integration --> displaylogs --> uploadlogs) so the diagram reflects when these steps run.
| log-path: | ||
| type: string | ||
| description: | | ||
| Path to a directory where the .txt logfiles for an integration testing run will be stored. | ||
| Relative and absolute file paths are both allowed. | ||
| Relative paths are rooted against the charm repository root. | ||
| Paths that begin with a wildcard character should be quoted to avoid being | ||
| interpreted as YAML aliases. | ||
| required: false | ||
| default: .logs |
There was a problem hiding this comment.
In the implementation of this log-path feature, the subsequent steps are intended to help debug failing integration tests. Ensure the log display + artifact upload steps are configured to run even when the integration test step fails (e.g., if: always()), and ensure the "show logs" loop does not fail the job when the directory exists but contains no matching *.txt files (bash will otherwise iterate the literal glob and cat will error).
| Relative and absolute file paths are both allowed. | ||
| Relative paths are rooted against the charm repository root. | ||
| Paths that begin with a wildcard character should be quoted to avoid being |
There was a problem hiding this comment.
The log-path description says relative paths are rooted against the "charm repository root", but the implementation later prepends inputs.charm-path for relative paths. Please clarify the wording so callers know whether the base is the repository root or the charm directory (charm-path).
| Relative and absolute file paths are both allowed. | |
| Relative paths are rooted against the charm repository root. | |
| Paths that begin with a wildcard character should be quoted to avoid being | |
| Relative and absolute file paths are both allowed. | |
| Relative paths are rooted against the charm directory specified by `charm-path` | |
| (which defaults to the repository root, `.`). | |
| Paths that begin with a wildcard character should be quoted to avoid being |
Change to
_charm-quality-checks.yaml(integration tests part) adding two steps:for each file found in
./.logs:the idea is, we can put our juju debug-log output in there, and the CI will display them and upload as artifact.
Context: canonical/pytest-jubilant#11
Tandem PR to demonstrate the workflow in action: https://github.com/canonical/o11y-tester-operator/actions/runs/16414197049